fix: refactor widget addition sagas and widget operation sagas to allow gridPosition in pasteSaga#32992
Conversation
github-actions
bot
added
Bug
rahulbarwal
removed
Bug
github-actions
bot
added
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8847157677. |
|
Deploy-Preview-URL: https://ce-32992.dp.appsmith.com |
…o fix/multiple-drag-drop-bb-canvas-resize-issue
jacquesikot
requested review from
a team and
marks0351
and removed request for
a team
WalkthroughThe changes primarily focus on enhancing the functionality and control flow associated with building blocks in the application, including new additions, modifications, and removals of functions and imports. These updates aim to streamline the process of adding and manipulating building blocks on the canvas, improving user experience and system performance. Changes
Assessment against linked issues
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
jacquesikot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (4)
app/client/src/sagas/BuildingBlocksSagas.ts (1)
10-10: Consider grouping related imports from the same module to improve readability.app/client/src/sagas/WidgetOperationUtils.ts (3)
Line range hint
1000-1020: Add error handling for potential null values in DOM operations.+ if (!stickyCanvasDOM) return;This addition ensures that the function does not attempt to access properties of
null, preventing runtime errors.
Line range hint
1022-1045: Define magic numbers as constants for better readability and maintainability.+ const DEFAULT_PADDING_MULTIPLIER = 2; let padding = ((LayoutWidget?.layoutSystemType === LayoutSystemTypes.AUTO ? AUTO_LAYOUT_CONTAINER_PADDING : CONTAINER_GRID_PADDING) + WIDGET_PADDING) * - 2; + DEFAULT_PADDING_MULTIPLIER;This change improves the readability of the code by replacing the magic number with a descriptive constant.
Line range hint
1047-1070: Add error handling for potential null values in DOM operations.+ if (!containerDOM || !canvasDOM) return;This addition ensures that the function does not attempt to access properties of
null, preventing runtime errors.
There was a problem hiding this comment.
@jacquesikot My review revolves around nomenclature. I feel all sagas in the new file have similar name and will confuse us in future.
|
@rahulbarwal @jacquesikot Do you think we can write a Jest unit test to assert that this problem won't show up again? |
|
Hey @mohanarpit We have a separate PR for automation in works, We will ensure this case is added there as well. |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
app/client/src/sagas/WidgetAdditionSagas.ts (1)
Line range hint
54-183: ThegetChildWidgetPropsfunction handles widget properties generation correctly. Consider adding more detailed comments in complex logic sections to improve maintainability.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8874414837. |
|
Deploy-Preview-URL: https://ce-32992.dp.appsmith.com |
jacquesikot
added
ok-to-test
Can you please link to that PR? Also any reason we are writing tests in a separate PR from the actual code? This way of working sounds like a violation of our coding best practices. |
|
@mohanarpit PR for cypress tests for this feature #33036 |
@jacquesikot Thanks. But my question still stands. Why is the PR for Cypress test separate from the change itself? Why are we not writing the code changes and Cypress tests in the same PR? This way, there is a changelog for our future selves about the code change & the test case to ensure that the code change doesn't regress. |
Nikhil-Nandagopal
added
Accelerators Product
github-actions
bot
added
QA
4 participants
Description
Tip
Problem
The current building block on canvas implementation uses the pasteSaga logic, and this logic requires the mousePosition to know where to paste the widgets. Due to building blocks being mimicked as a single widget, we get grid position values instead of mouse location when dragging and dropping building blocks, this forced us to create e utility function that generates mousePosition from gridPosition values. This logic was flawed and led to building blocks placement being wrong when the view is scrolled.
Solution
We have decided to implement the pasteSaga to allow both mousePosition and gridPosition as expected values for dropping the copied widgets unto the canvas.
Fixes #32962
Automation
/ok-to-test tags="@tag.Widget"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8875384318
Commit: 6f425a3
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores